Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NUKE leftover CPU lightGrid code, minor fixes and cleanup #1496

Merged
merged 4 commits into from
Jan 21, 2025

Conversation

VReaperV
Copy link
Contributor

  • NUKE some code that was used before lightGrid was moved to the GPU, and which was now just waiting resources.
  • Made r_forceAmbient (hopefully) work by actually using it with the GLSL lightGrid.
  • r_forceAmbient and r_ambientScale changed to new-style cvars, +some cleanup.

@VReaperV VReaperV force-pushed the nuke-unused-cpu-lightgrid branch 2 times, most recently from 28a73d5 to 49d98ef Compare January 18, 2025 19:42
Copy link
Member

@illwieckz illwieckz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it WFY, LGTM.

@illwieckz
Copy link
Member

illwieckz commented Jan 20, 2025

Speaking of alternate lightgrid code, @VReaperV do you think it's technically doable to have a “lowest” lightgrid implementation (only meaningful for models), that would just sample the lightgrid color at the origin of the model (CPU side), and uniformly lit the whole model with that color (probably using the rgbgen fields), skipping entirely the lightgrid GLSL, relying on the fullbright GLSL code?

@VReaperV
Copy link
Contributor Author

Speaking of alternate lightgrid code, @VReaperV do you think it's technically doable to have a “lowest” lightgrid implementation (only meaningful for models), that would just sample the lightgrid color at the origin of the model (CPU side), and uniformly lit the whole model with that color (probably using the rgbgen fields), skipping entirely the lightgrid GLSL, relying on the fullbright GLSL code?

Yeah, that would be easily doable. In fact, particles and trails already do pretty much that (but they interpolate between a few points) - which is kinda stupid, because it actually involves an IPC round-trip for no reason...
Why do the uniform lighting for models though?

{
sscanf( value, "%f %f %f", &tr.worldEntity.ambientLight[ 0 ], &tr.worldEntity.ambientLight[ 1 ],
&tr.worldEntity.ambientLight[ 2 ] );
if ( r_forceAmbient.Get() == 0 ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If r_forceAmbient is overriding some value specified by the BSP, shouldn't the cvar be off by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps. I just used the value that was already there for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's also the issue that there could be maps relying on it.

@illwieckz
Copy link
Member

Why do the uniform lighting for models though?

Just as an ugly and cheap alternative for the “lowest” preset that would not be a cheat, because the model would still be dark in a dark room and lit in a lit room. It would save on GLSL processing and on GPU memory, and not involve 3D textures in GPU memory. Just a cheap alternative that would not be a cheat.

We do this in GLSL now, and the results of this code are not actually used for anything.
These will now actually modify the lightGrid.
@VReaperV VReaperV force-pushed the nuke-unused-cpu-lightgrid branch from 49d98ef to c6d6122 Compare January 20, 2025 18:02
@VReaperV
Copy link
Contributor Author

(rebased)

@VReaperV
Copy link
Contributor Author

I'm merging it as is, if there are further concerns for the default value of r_forceAmbient, they can be fixed in a different pr, since it's just a single number change. This pr does not modify the default value, nor its range from what we already had (with reasoning outlined in the review).

@VReaperV VReaperV merged commit 5a071e2 into DaemonEngine:master Jan 21, 2025
9 checks passed
@VReaperV
Copy link
Contributor Author

Why do the uniform lighting for models though?

Just as an ugly and cheap alternative for the “lowest” preset that would not be a cheat, because the model would still be dark in a dark room and lit in a lit room. It would save on GLSL processing and on GPU memory, and not involve 3D textures in GPU memory. Just a cheap alternative that would not be a cheat.

You might be able to just use

int R_LightForPoint( vec3_t point, vec3_t ambientLight, vec3_t directedLight, vec3_t lightDir )

Interpolating probably shouldn't be too much of an issue? If it is, just simply getting the closest point can be done easily enough.

@VReaperV VReaperV deleted the nuke-unused-cpu-lightgrid branch January 21, 2025 00:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants